Test queryable built-in role synchronization #118964
Conversation
slobodanadamovic
added
>test
| final Map<String, String> indexedRolesDigests = readIndexedBuiltInRolesDigests(clusterService.state()); | ||
| if (roles.rolesDigest().equals(indexedRolesDigests)) { | ||
| logger.debug("Security index already contains the latest built-in roles indexed, skipping synchronization"); | ||
| return; |
There was a problem hiding this comment.
This is a bug fix where this method would return without calling synchronizationInProgress.set(false);.
slobodanadamovic
added
v9.0.0
v8.18.0
auto-backport
|
Pinging @elastic/es-security (Team:Security) |
| }, QueryableBuiltInRolesSynchronizer::handleException))); | ||
| } | ||
| } finally { | ||
| final Map<String, String> indexedRolesDigests = readIndexedBuiltInRolesDigests(clusterService.state()); |
There was a problem hiding this comment.
I know this is getting to be a lot but I wonder if we want a try-catch around this whole block after all -- readIndexedBuiltInRolesDigests can technically throw and so can executor.execute.
I spent a bit of time thinking of how to refactor this to avoid four separate calls to synchronizationInProgress.set(false) but nothing straight-forward comes to mind.
There was a problem hiding this comment.
Absolutely, I'll wrap it with try-catch block. I don't see other option.
| @@ -452,6 +453,14 @@ static class MarkRolesAsSyncedTask implements ClusterStateTaskListener { | |||
| this.newRoleDigests = newRoleDigests; | |||
| } | |||
|
|
|||
| public Map<String, String> getExpectedRoleDigests() { | |||
There was a problem hiding this comment.
Don't think this is used anywheres
|
|
||
| private DiscoveryNodes mixedVersionNodes() { | ||
| VersionInformation oldVersion = new VersionInformation( | ||
| VersionUtils.randomCompatibleVersion(random(), VersionUtils.getPreviousVersion()), |
There was a problem hiding this comment.
TIL VersionUtils.getPreviousVersion().isCompatible(Version.CURRENT) is true so we'll get failures for testMixedVersionsCluster -- so I think we want randomVersionBetween here instead
There was a problem hiding this comment.
Good catch!
| any() | ||
| ); | ||
| verify(clusterService, times(2)).state(); | ||
| verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); |
There was a problem hiding this comment.
Might also make synchronizationInProgress pkg-private (or a getter to it) and assert on it here: assertThat(synchronizer.synchronizationInProgress.get(), equalTo(false));
…ng-queryable-built-in-roles # Conflicts: # muted-tests.yml
slobodanadamovic
added
the
auto-merge-without-approval
💔 Backport failed
You can use sqren/backport to manually backport by running |
Adds more tests for built-in roles synchronization, and fixes a bug where
synchronizationInProgresshasn't been reset properly.Resolves #118806